Thanks for kicking off the debate about those IAM policies! 👍
I'm not sure that introducing a de-facto new, custom syntax for the whole policy is worth it though.

I was wondering if a custom JSON unmarshal function would do?

type IAMPolicy struct {
    Statement []IAMPolicyStatement
    Version   string
}

type IAMPolicyStatement struct {
    Sid          string                         `json:",omitempty"`
    Effect       string                         `json:"-"`
    Principal    IAMPolicyStatementPrincipal    `json:",omitempty"`
    NotPrincipal IAMPolicyStatementPrincipal    `json:",omitempty"`
    Action       []string                       `json:",omitempty"`
    NotAction    []string                       `json:",omitempty"`
    Resource     []string                       `json:",omitempty"`
    NotResource  []string                       `json:",omitempty"`
    Condition    map[string]map[string][]string `json:",omitempty"`
}

type IAMPolicyStatementPrincipal struct {
    AWS           []string `json:",omitempty"`
    CanonicalUser []string `json:",omitempty"`
    Federated     []string `json:",omitempty"`
    Service       []string `json:",omitempty"`
}

There isn't such type available in AWS SDK unfortunately - this is what I collected from docs:
http://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements.html

It could be even more granular and translate Statement[].Condition into structs for easier manipulation, but it depends how easy it's going to be to implement. There's 28 conditions supported according to the PolicyGen.

Then we would just need to use normalizeIAMPolicy (very similar to normalizeJSON) as a StateFunc for relevant attributes.

What do you think?

I am personally attracted to having a policy doc syntax that fits into the Terraform syntax better, plays better with the interpolation of lists, etc. Substituting values into a JSON string works after a fashion, but it leads to pretty unhelpful diffs and requires special attention to escaping, etc.

While I can see the argument that this invents effectively a new serialization of policy documents, I'd argue that every Terraform provider is inventing a new syntax for something, and this optional helper improves the user experience for cases where the policy document has some dynamic elements while not hurting other use-cases that benefit more from a static policy file on disk, or an inline literal heredoc.

Having the separate normalization on the policy JSON properties themselves would still be useful in addition, since there are multiple different ways to write the same thing in IAM policies and we should diff those well even if this optional helper resource is not in use.

I'm not sure I'd go so far as to explicitly model the condition operators and the policy statement principal types. They both seem like sets that will grow and shrink as the AWS feature offering changes, and so it would be nice not to lock Terraform into the set of options available today.

As a sidebar: I #3084 ended up adding a very similar normalizeJSON function for Chef attributes. Seems likely that this one is going to end up rather common given that JSON is used for a bunch of different apps, so maybe it's worth promoting that to either core or the schema helper for better reuse?

I am personally attracted to having a policy doc syntax that fits into the Terraform syntax better, plays better with the interpolation of lists, etc.

I see, that's reasonable.

I'm just thinking whether we're not going in a direction where we'll have aws_iam_policy_document_statement, aws_iam_policy_document_statement_condition etc., which feels a bit messy between all existing resources which are essentially just wrapping an API endpoint.

Maybe it's just a matter of better separation in docs and elsewhere... or maybe it's just me...

The usual reason to split up a concept across multiple resources is to allow it to be decomposed across multiple different Terraform modules, to share a higher-level concept with several lower-level modules using remote state.

Thinking through use-cases here the only thing I could see a practical use-case for is sharing policy statements across multiple policies. I actually considered such a thing when I was implementing this, and considered handling it by allowing this one aws_iam_policy_document resource to take an array of other policy document JSONs and merge their statements into the resulting statement set, allowing composition of statements without creating a new top-level concept. In this case you're re-using statements sets (in the form of policy documents) rather than individual statements themselves.

I left it out of this first iteration since I figured there'd be some discussion about this, but I don't think it would be very difficult to implement such a thing as long as the mechanism is clearly defined in the documentation so users can predict what it will do.

Re-using objects within statements probably has some edge cases where it would be useful, but the nested items are small and often very contextual, so I wouldn't expect that to arise so often. Having a general feature for reusing fragments of resource configuration does seem to arise occasionally, e.g. sharing AWS tags across resources, but I'm not sure that more resources is the answer to that.

This syntax for specifying policy documents is really desirable. 👍

I would love to get this out, as we're looking to pull these policy docs into Terraform now, and the current heredoc syntax is clunky.

As discussed in #3519, different AWS services normalize the IAM policy structures in different ways, so this PR alone can't totally address the normalization problem. However, I think there is a path forward:

• Introduce this new resource as a way to generate "Terraform-normalized" IAM policies, which conform to the IAM policy docs but are minimal: they don't include any property whose absense means the same thing as being empty.
• Each separate resource that consumes IAM policies should have a StateFunc on its policy attribute(s) that parses the provided JSON (which may or may not come from this aws_iam_policy_document resource) and does any specific normalization that the given resource requires. For example, for aws_vpc_endpoint.policy we apparently need to add a top-level Version, defaulting to "2008-10-17", and make sure there's a Sid inside the statement even if it's the empty string.

@apparentlymart you should speak to @jen20 on this - he was building an aws_policy provider for Terraform that may be of use here :)

Attached a PR to address some behaviour that I am seeing in S3 bucket policies:

• Sid is set to a empty value if it was not specified
• Action is truncated to a single string if it is a single-entry array
• Multiple Actions are sorted alphabetically

These are some things that may need to be considered when making a custom unmarshaler. @apparentlymart mentioned similar gotchas.

Circling back around to this. Reviewing the thread and the diff it seems like barring any remaining objections from @radeksimko or @jen20 this is good to go.

You two want to weigh in?

I'm in favour of having such resource after @apparentlymart explained the benefits nicely in this thread, however I'd like to see this PR as a seed for @vancluever 's PR #4278 and many others which may follow.

This is why I'd rather see json (un)marshallers being used as that will make reusability of such solution possible and also should make maintenance of the code easier if AWS decides to add/remove/modify any part of the syntax.

I'm sorry I missed this comment before:

As discussed in #3519, different AWS services normalize the IAM policy structures in different ways, so this PR alone can't totally address the normalization problem.

that said I still believe that structs with mappings would make future maintenance much easier (as opposed to for-loops and maps).

I have rebased this and rewritten it to use structs rather than raw map[string]interface{} as @radeksimko suggested.

With that said, we've waited this long for this and #4961 (data sources) seems to be on the way so I'm thinking we wait on this one a little longer and then rework it as a data source before merging it.

Merged as #6881!

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.